Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an AEAD encrypting transformer for storing secrets encrypted at rest #41939

Merged

Conversation

smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Feb 23, 2017

Tweak the ValueTransformer interface slightly to support additional
context information (to allow authenticated data to be generated by the
store and passed to the transformer). Add a prefix transformer that
looks for known matching prefixes and uses them. Add an AES GCM
transformer that performs AEAD on the values coming in and out of the
store.

Implementation of https://docs.google.com/document/d/1lFhPLlvkCo3XFC2xFDPSn0jAGpqKcCCZaNsBAv8zFdE/edit# and #12742

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 23, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: smarterclayton

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Feb 23, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 23, 2017
@smarterclayton smarterclayton force-pushed the encrypt_transformer branch 2 times, most recently from 80bc935 to a3ccbba Compare February 23, 2017 00:29
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2017
@timothysc
Copy link
Member

@smarterclayton did you want this for 1.6?

@timothysc timothysc added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Feb 24, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2017
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 28, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2017
@timothysc timothysc removed their assignment May 16, 2017
Copy link
Member

@destijl destijl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, would be good to get this in, we have someone starting on key management and config.

Tweak the ValueTransformer interface slightly to support additional
context information (to allow authenticated data to be generated by the
store and passed to the transformer). Add a prefix transformer that
looks for known matching prefixes and uses them. Add an AES GCM
transformer that performs AEAD on the values coming in and out of the
store.
Adds context argument which must be set for AES GCM authenticated data
to be passed.
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 17, 2017

@smarterclayton: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins GCE e2e cc89981a30a6078c8b5aa0af6bf2b4935c5c9cbe link @k8s-bot cvm gce e2e test this
Jenkins GCI GCE e2e cc89981a30a6078c8b5aa0af6bf2b4935c5c9cbe link @k8s-bot gci gce e2e test this
Jenkins non-CRI GCE e2e cc89981a30a6078c8b5aa0af6bf2b4935c5c9cbe link @k8s-bot non-cri e2e test this
Jenkins non-CRI GCE Node e2e cc89981a30a6078c8b5aa0af6bf2b4935c5c9cbe link @k8s-bot non-cri node e2e test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@smarterclayton
Copy link
Contributor Author

@k8s-bot kops aws e2e test this

@smarterclayton
Copy link
Contributor Author

Note that we can always add new transformer implementations at any time - this is intentionally flexible.

@smarterclayton smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2017
@smarterclayton
Copy link
Contributor Author

Applying label as per comment - this doesn't activate this code, so subsequent PRs can easily accommodate requests for change.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 45709, 41939)

@k8s-github-robot k8s-github-robot merged commit 6047143 into kubernetes:master May 17, 2017
if err != nil {
b.Fatal(err)
}
// reverse the key order if stale
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smarterclayton I don't see in the code how order is changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, will add in a follow up. Just a benchmark.


// TransformToStorage uses the first transformer and adds its prefix to the data.
func (t *prefixTransformers) TransformToStorage(data []byte, context Context) ([]byte, error) {
transformer := t.transformers[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that the transformers is nil here? Should we add a check for that case in NewPrefixTransformers()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally don't guard against passing nil to constructors since it's coding error to get to that point.


func TestPrefixFrom(t *testing.T) {
testErr := fmt.Errorf("test error")
transformErr := fmt.Errorf("test error")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having two different errors with the same message is confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The equality test is different.

"k8s.io/apiserver/pkg/storage"
"k8s.io/apiserver/pkg/storage/etcd3"
"k8s.io/apiserver/pkg/storage/storagebackend"
"k8s.io/apiserver/pkg/storage/value"
)

func newETCD3Storage(c storagebackend.Config) (storage.Interface, DestroyFunc, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the similar change for the newETCD2Storage() that still use hard-coded etcd.IdentityTransformer. Was it on purpose?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here, any plan to support etcd2?

ixdy pushed a commit to ixdy/kubernetes that referenced this pull request Jun 5, 2017
…former

Automatic merge from submit-queue (batch tested with PRs 46550, 46663, 46816, 46820, 46460)

Add configuration for encryption providers

## Additions

Allows providing a configuration file (using flag `--experimental-encryption-provider-config`) to use the existing AEAD transformer (with multiple keys) by composing mutable transformer, prefix transformer (for parsing providerId), another prefix transformer (for parsing keyId), and AES-GCM transformers (one for each key). Multiple providers can be configured using the configuration file.

Example configuration:
```
kind: EncryptionConfig
apiVersion: v1
resources:
  - resources:
    - namespaces
    providers:
    - aes:
        keys:
        - name: key1
          secret: c2vjcmv0iglzihnly3vyzq==
        - name: key2
          secret: dghpcybpcybwyxnzd29yza==
    - identity: {}
```

Need for configuration discussed in:
kubernetes#41939
[Encryption](https://github.com/destijl/community/blob/3418b4e4c6358f5dc747a37b90a97bc792f159ee/contributors/design-proposals/encryption.md)

**Pathway of a read/write request**:
1. MutableTransformer
2. PrefixTransformer reads the provider-id, and passes the request further if that matches.
3. PrefixTransformer reads the key-id, and passes the request further if that matches.
4. GCMTransformer tries decrypting and authenticating the cipher text in case of reads. Similarly for writes.

## Caveats
1. To keep the command line parameter parsing independent of the individual transformer's configuration, we need to convert the configuration to an `interface{}` and manually parse it in the transformer. Suggestions on better ways to do this are welcome.

2. Flags `--encryption-provider` and `--encrypt-resource` (both mentioned in [this document](https://github.com/destijl/community/blob/3418b4e4c6358f5dc747a37b90a97bc792f159ee/contributors/design-proposals/encryption.md) ) are not supported in this because they do not allow more than one provider, and the current format for the configuration file possibly supersedes their functionality.

3. Currently, it can be tested by adding `--experimental-encryption-provider-config=config.yml` to `hack/local-up-cluster.sh` on line 511, and placing the above configuration in `config.yml` in the root project directory.

Previous discussion on these changes:
sakshamsharma#1

@jcbsmpsn @destijl @smarterclayton

## TODO
1. Investigate if we need to store keys on disk (per [encryption.md](https://github.com/destijl/community/blob/3418b4e4c6358f5dc747a37b90a97bc792f159ee/contributors/design-proposals/encryption.md#option-1-simple-list-of-keys-on-disk))
2. Look at [alpha flag conventions](https://github.com/kubernetes/kubernetes/blob/master/pkg/features/kube_features.go)
3. Need to reserve `k8s:enc` prefix formally for encrypted data. Else find a better way to detect transformed data.
k8s-github-robot pushed a commit that referenced this pull request Aug 26, 2017
Automatic merge from submit-queue (batch tested with PRs 49850, 47782, 50595, 50730, 51341)

Fix benchmarks to really test reverse order of the keys

**What this PR does / why we need it**:
This PR modifies the code to do what comments says -- reverse the order of keys. It also fixes the logic that was wrong and didn't allow stale data.

**Special notes for your reviewer**:
This change resolves the following review comments:
- #41939 (comment)
- #46916 (comment)
- #46916 (comment)

**Release note**:
```release-note
NONE
```

PTAL @smarterclayton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants